Add _quantity property for values with units #38
Conversation
There was a problem hiding this comment.
oooh, lovely! I think this is a great intermediate step, before (/regardless of whether) we remove the unit field itself. I've never used pint before... this took much less code than I expected
what would you think of adding a setter to that quantity_property that accepts a pint.Quantity and extracts the magnitude and units? (not sure how hard that is for the few custom units you had to define here)
|
A setter would work but again there wouldn't be any great way to type that. Mutable fields raises some other issues too -- I'll file another issue to discuss that. |
|
Oh, this is timely! The Pint main developer filed an issue to discuss annotations just the other day: hgrecco/pint#1166 |
Codecov Report
@@ Coverage Diff @@
## master #38 +/- ##
=======================================
Coverage 95.20% 95.20%
=======================================
Files 1 1
Lines 396 480 +84
=======================================
+ Hits 377 457 +80
- Misses 19 23 +4
Continue to review full report at Codecov.
|
|
I added one test to get a feel for what we might want to check. I didn't use the model-as-fixture approach since we saw that introduced problems in #34. I originally tried specifying those two length values in different units, but I ran into float precision issues due to the way Pint manages conversions internally (the difference would end up being like 54.99999999999993 nm instead of exactly 55). Pint does support building the UnitRegistry with Decimal factors instead of float which would yield exact values, but then all Quantities would need to be initialized with int or Decimal values and users would need to convert magnitudes to float before doing any math with other floats (Decimals were deliberately designed not to interoperate with floats to avoid silent loss of precision). I can cook something up to make this work in the test (like comparing with a tolerance or rounding first) but I wonder if users are going to be confused if they can't convert e.g. 123 nm to pm and get 123000 exactly. >>> ureg.Quantity(123, 'nm').to('pm').m
123000.00000000001Maybe this isn't a big issue in practice and I'm overthinking it. Thoughts? |
yeah, it's slightly annoying... but I think it might be more annoying to have to use Decimal objects everywhere. so it's fine with me to leave this as a "maybe someday if we or someone else complains" and move on for now. rounding for tests purposes works for me |
|
Does this look like enough tests? I wanted to avoid testing too much and end up just testing Pint itself. Also do you feel |
This still needs tests but I wanted to get your thoughts first. I didn't want to mess with the value fields and
..._unitfields so I added a parallel property with the_quantitysuffix for all fields with units.Ultimately it would be cleaner to replace the value fields with a
pint.Quantityand hide the_unitfields completely (that approach would work especially well for create/modify operations), but I'm not sure how we'd want to do the type annotations in that case. Pint doesn't play so well with typing as it loads unit definitions at runtime -- all you can do is type stuff asQuantity. We can definitely get Pydantic to parse quantities and reject incompatible units, but mypy probably won't be any help.closes #3